-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
boost: add boost/1.76.0 + bump b2 #5246
Conversation
This comment has been minimized.
This comment has been minimized.
I detected other pull requests that are modifying boost/all recipe:
This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
This comment has been minimized.
This comment has been minimized.
@jgsogo @uilianries
So I believe this is a CI infrastructure problem. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I'd of sworn I've seen newer features being used! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1.74.0 failure remains a mystery to me 😄 |
Odd it passed before.... might be a bug that's been hiding! Last last retry this time I feel it! |
“Insanity is doing the same thing, over and over again, but expecting different results.” |
This comment has been minimized.
This comment has been minimized.
Ok, it's official. |
@@ -139,6 +139,9 @@ def export(self): | |||
@property | |||
def _min_compiler_version_default_cxx11(self): | |||
# Minimum compiler version having c++ standard >= 11 | |||
if self.settings.compiler == "apple-clang": | |||
# For now, assume apple-clang will enable c++11 in the distant future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 12 is 14 by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to say "c++11 by default"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! It's our usual problem... AFAIK, in apple-clang 12 they finally set it to c++14
by default (they skipped 11)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the (failed) build on apple-clang 12, it does not have (at least) c++11 support by default.
See https://www.phoronix.com/scan.php?page=news_item&px=EXT4-Casefolding-With-Encrypt
Maybe it is just a matter of the number of jobs we are running in parallel inside each machine. We might be hitting some memory limits when running these big builds in parallel. 🤔 I mean, instead of parsing the logs and try to understand the errors (something I would love to do), it easier to run less jobs in parallel in each windows machine. From the user perspective, it can be much better even if it takes a little bit more time, because the alternative is a failure plus close/open pull-request. |
This particular error has been on the rise, even for medium size projects. I think reducing the degree of parallelism is acceptable, most of the time I switch tasks while waiting for results 🙈 |
Maybe noteworthy is that it isn't the boost build that was failing in the last builds, but the test package. Perhaps lowering the parallel level of building the tests will avoid the unexpected process kills? |
All green in build 12 (
|
@@ -247,6 +263,28 @@ def config_options(self): | |||
# Shared builds of numa do not link on Visual Studio due to missing symbols | |||
self.options.numa = False | |||
|
|||
if tools.Version(self.version) >= "1.76.0": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably, in future we should add C++ standard requirements to the yml (existing or new), as different libraries tend to change their requirements across boost releases
@prince-chrismc @SpaceIm Can I get some reviews please? |
min_compiler_version = self._min_compiler_version_default_cxx11 | ||
if min_compiler_version is None: | ||
self.output.warn("Assuming the compiler supports c++11 by default") | ||
elif tools.Version(self.settings.compiler.version) < min_compiler_version: | ||
disable_math() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not inject C++11 flag instead if math is enabled? I understand that math will be disabled if compiler default standard is less than C++11, even if it supports C++11, unless compiler.cppstd is explicitly set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's right thing to do. if compiler.cppstd is set explicitly, it should be passed according to the setting. if it's not defined at all, it means just follow the compiler's default, whatever it is. in that case, injecting standard different from the default will only cause confusion, or even problems (as boost changes ABI for C++11 which might be undesired).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what we do indirectly in almost all libs based on CMake, since usually they force CXX_STANDARD to the minimum standard required to properly build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that this won't cause an influx of issues reporting missing boost math libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we're doing it now, but it doesn't mean we're doing the right thing :)
I think the plan is to eventually add cppstd
support for CCI and process it accordingly, instead of silently injecting -std=c++xx
in some recipes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't inject this flag ourself, it's already there usually in underlying build files of libraries, this will require too much patches. I think that the pressure to recipes maintainers is highly underestimated for this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for boost, the flag is controlled by b2 cxxstd=<version>
and it's not hard-coded. so for us, it should be a simple mapping compiler.cppstd
<-> cxxstd
.
for other builds systems, it will be another option (e.g. CMAKE_CXX_STANDARD
in CMake or cpp_std
in meson), thus similar mapping will happen in the recipe or build helper.
some projects tend to hard code specific C++ standard in their makefiles (e.g. hardcoding CXXFLAGS=-std=c++<version>
or set(CMAKE_CXX_STANDARD <version>
). should we do something about it?
there are two ways:
- provide patches to remove hardcoded C++ standard flags
- assume the hard-coded C++ standard is the only one supported by the library, thus, throw
ConanInvalidConfigurations
for all othercompiler.cppstd
values besides hard-coded one
I personally think the second approach is the most correct one.
that we're doing right now is a complete mess, and the only excuse is that CCI doesn't currently support compiler.cppstd
, and if we try to do things correctly, we will be unable to build many libraries.
but so far it works fine, and we had almost no reports of it caused any actual problems.
doing things correctly, on other hand, will increase pressure on maintainers, as you pointed out, as well it will make CI more complicated and will require way more resources (and will make builds even slower, multiply boost build time by the number of active C++ standards).
probably, the benefit doesn't worth the effort right now, and there are other areas which will bring more benefit for much less effort. still, in future, some C++ standard may introduce a huge ABI break, which will make "correct" handling of C++ standard in CCI a vital and must have feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think the second approach is the most correct one.
This is the policy we have, before CMake was the main build script it was a wild west and it's the on sensible way...
As a consumer if you build locally you expect the project to be configured as intended. It's not our place to change that value... nor test all possible combinations and patch in between cppstd
@@ -16,7 +16,7 @@ | |||
except ImportError: | |||
from io import StringIO | |||
|
|||
required_conan_version = ">=1.28.0" | |||
required_conan_version = ">=1.33.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
required_conan_version = ">=1.33.0" | |
required_conan_version = ">=1.32.0" |
even lower ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CCI policy is to require latest conan version, so 1.33 is okay, no need to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I prefer to avoid triggering a rebuild for this trivial change 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not very important. But I disagree, required_conan_version
is a useful information. It avoids cryptic errors for consumers. Moreover, think to beginners, conan is already complex, so let be honest, few people read the all documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's requiring conan 1.33.0 now. Isn't that fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But sure, if this is the only modification, let's skip it since it takes 4 of 5 hours to build (which is not too long actually for so many versions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest problem is that the build infrastructure is a bit unstable when building boost. I needed to trigger a few rebuilds before it could complete.
def disable_math(): | ||
super_modules = self._all_super_modules("math") | ||
for smod in super_modules: | ||
try: | ||
setattr(self.options, "without_{}".format(smod), True) | ||
except ConanException: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to be sure to understand:
- super modules is the complete list of modules which might have math in their own dependency graph?
- what could fail in
setattr(self.options, "without_{}".format(smod), True)
? Is it not under control?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to be sure to understand:
* super modules is the complete list of modules which might have math in their own dependency graph?
Yes, the current implementation is quick and dirty and I don't guarantee it working under all combinations though.
* what could fail in `setattr(self.options, "without_{}".format(smod), True)`? Is it not under control?
The dependencies
section of the dependencies-x.yy.yml
file contains modules that are not valid options of boost.build (configure_options
section). So I just ignore these.
* boost: bump b2 build dependency * boost: add boost/1.76.0 * boost: add 1.76.0 to config.yml * boost: move check to validate() + Boost.Math requires c++11 * boost: lower minimum required conan version * boost: remove dead comment * boost: assume apple-clang will never implement c++11
Specify library name and version: boost/1.76.0
Fixes #5272
Release notes: https://www.boost.org/users/history/version_1_76_0.html
No new libraries!
conan-center hook activated.